IBX-6773: Fixed loading Bookmarks for non-accessible content items by vidarl · Pull Request #476 · ibexa/core

vidarl · 2025-02-11T14:17:47Z

Question	Answer
JIRA issue	IBX-6773
Type	bug
Target Ibexa version	`v4.6`
BC breaks	yes

Related PRs:

IBX-6773: Aligned Bookmarks loadBookmarks call with Repository filtering requirements admin-ui#1835

Description:

If user bookmark some location which he later looses access too, then the bookmark list in admin-ui fails with an exception.
Simply fixing BookmarkService::loadBookmarks() would be easy. The problem is to implement countUserBookmarks() in persistence layer and having it taking into account user permissions so that BC would be kept.

Talked with Adam on how to solve this without breaking BC and he suggested implementing it using filtering

The Bookmark filter will only work with location filtering (LocationService::find()), not with content (ContentService::find())

This is a port of ezsystems/ezplatform-kernel#408 which was not approved and merge in time before 3.3 went EOL.

For QA:

Read ticket for info on how to reproduce

Documentation:

Documentation for the new filter needs to be made, indeed

sonarqubecloud · 2025-02-12T12:26:48Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php

src/lib/Repository/BookmarkService.php

tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php

phpstan-baseline.neon

src/contracts/Persistence/Bookmark/Handler.php

konradoboza

Did you make sure none of the deprecated classes are still in use?

vidarl · 2026-02-24T11:44:12Z

@konradoboza : FYI : Had to add related PR : ibexa/admin-ui#1835

konradoboza · 2026-02-25T14:43:33Z

tests/integration/Core/Repository/LocationServiceTest.php

+        $this->assertEquals($contactUsLocationId, $afterSwap->items[0]->id);
+        $this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id);


Why this change? It's not clear at first glance.

@konradoboza
The testcase initially tests swapping two locations that are both bookmarked.
I modified the test to check behavior when 1 location is bookmarked and the other is not

…n - PHPStand and BookmarkQueryBuilder

…n - PHPStand and Bookmark\IdSortClauseQueryBuilder

…n - Added more Strict typing

…tion/BookmarkQueryBuilderTest.php Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

alongosz · 2026-02-27T20:09:50Z

PR rebased.

konradoboza

Can we have some integration test scenarion within Ibexa\Tests\Integration\Core\Repository\BookmarkServiceTest?

tests/integration/Core/Repository/LocationServiceTest.php

src/lib/Repository/BookmarkService.php

vidarl · 2026-03-10T09:13:08Z

Did you make sure none of the deprecated classes are still in use?

@konradoboza : Yeah, did a search and found nothing except the use in test:
core/tests/lib/Persistence/Cache/BookmarkHandlerTest.php
core/tests/lib/Persistence/Legacy/Bookmark/Gateway/DoctrineDatabaseTest.php
core/tests/lib/Persistence/Legacy/Bookmark/HandlerTest.php

Should those tests be marked deprecated somehow to ?

konradoboza · 2026-03-10T09:15:06Z

Not at all. We just change the usage of the method being deprecated from day one. Tests will be adjusted accordingly on the actual removal of the code in question.

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

konradoboza · 2026-03-10T09:50:32Z

#476 (review) please take also this into account.

…ervice::count()

vidarl · 2026-03-10T12:03:55Z

Can we have some integration test scenarion within Ibexa\Tests\Integration\Core\Repository\BookmarkServiceTest?

Those integration tests uses BookmarkService. And the BookmarkService is already updated to use the filter instead of the deprecated functions from persistence layer.

But I added also a test for count ( BookmarkServiceTest::testCountBookmarks() ) in 5fd0eb2

Edit: with a fixup : 993e850

@konradoboza

…cationService::count()

tests/integration/Core/Repository/BookmarkServiceTest.php

…cationService::count()

tests/integration/Core/Repository/BookmarkServiceTest.php

… and LocationService::count()

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

sonarqubecloud · 2026-03-10T13:45:09Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

alongosz

@vidarl I'm very sorry for the delayed review.
+1 for the idea of using Filtering for this, but I have concerns about API design and some other minor remarks:

alongosz · 2026-03-11T10:01:15Z

src/contracts/Persistence/Bookmark/Handler.php

    /**
     * Loads bookmarks owned by user.
     *
+     * @deprecated The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.


Deprecating it in 6.0 is fine for now, but the annotation format is not correct and most likely it's gonna affect or even crash phpDocumentor ;-)
Please see PHP Conventions: Deprecation standards.

alongosz · 2026-03-12T00:30:30Z

src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php

+ * Supported operators:
+ * - EQ: matches against a unique user id
+ */
+final class IsBookmarked extends Criterion implements FilteringCriterion


Extending criterion means that this would be available not just for filtering but for search too. Hence it would require implementing handlers for all supported search engines.

Filtering was supposed initially to mirror search service, so there are no examples of Filtering Criteria that are not Search Criteria at the same time, but the intention of Filtering was that it can exist as a separate concepts for data which are not supposed to be a part of Search Index but a database.

ATM it's possible to use it with search and get the following fatal error at runtime:

Ibexa\Contracts\Core\Repository\Exceptions\NotImplementedException : Intentionally not implemented: No visitor available for: Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked with operator =

If it doesn't rely on Criterion, the error should appear on at least static analysis level, which provides better DX.

alongosz · 2026-03-12T00:31:47Z

src/lib/Persistence/Cache/BookmarkHandler.php

+     * @deprecated The "BookmarkHandler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
     * {@inheritdoc}


Please don't repeat the same message here. It should be present on the interface only.
In fact, you can drop the entire block because doc inheritance is done by default (it's legacy annotation here).

alongosz · 2026-03-12T00:32:06Z

src/lib/Persistence/Legacy/Bookmark/Gateway/DoctrineDatabase.php

@@ -146,6 +148,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
    }

    /**
+     * @deprecated The "DoctrineDatabase::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *


Same remark here

alongosz · 2026-03-12T00:32:32Z

src/lib/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php

@@ -66,6 +71,9 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
        }
    }

+    /**
+     * @deprecated The "ExceptionConversion::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.


alongosz · 2026-03-12T00:33:20Z

src/lib/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php

+    /**
+     * @deprecated The "ExceptionConversion::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
+     * @return array


This PHPDoc return doesn't provide any extra value. It should be documented as at least array<string, mixed> but on the interface (or abstract) level.

alongosz · 2026-03-12T00:34:46Z

src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php

+ */
+final class IsBookmarked extends Criterion implements FilteringCriterion
+{
+    public function __construct(int $value)


Important
API design remark

Typically you'd want here bool $isBookmarked = true. Semantically IsBookmarked(14) doesn't read well and is rather not expected with the verb Is.

User ID value should be part of a handler (query builder), not a criterion. That being said, I see you provided a case for IsBookmark of user X or user Z. It's a bit out of scope of the usage of this criterion, but if you indeed want such flexibility, user ID should be an optional 2nd argument (by default null, taken from current user in query builder).

alongosz · 2026-03-12T00:38:29Z

src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php

+            );
+
+        /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */
+        $value = $criterion->value;


As described earlier, this should be part of the logic here, not user input by default. Inject PermissionResolver and get current user ID

alongosz · 2026-03-12T00:41:24Z

src/lib/Repository/BookmarkService.php


+        $list = new BookmarkList();
+        $list->totalCount = $result->totalCount;
+        $list->items = $result->locations;


locations is a magic property, which using of should be avoided due to performance. You should rather do iterator_to_array here.

alongosz · 2026-03-12T12:55:02Z

src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php

+        /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */
+        $value = $criterion->value;
+
+        if (\is_array($value)) {
+            if (!isset($value[0])) {
+                throw new \InvalidArgumentException('IsBookmarked criterion value must contain userId at index 0.');
+            }
+            $value = $value[0];
+        }
+
+        return $queryBuilder->expr()->eq(
+            'bookmark.user_id',
+            $queryBuilder->createNamedParameter(
+                (int)$value,
+                ParameterType::INTEGER
+            )
+        );


Following the API design remark, This could be either:

$userId = $criterion->getUserId() ?? $this->permissionResolver->getCurrentUserReference()->getUserId();

or simply

$userId = $this->permissionResolver->getCurrentUserReference()->getUserId();

PermissionResolver should be injected via constructor.

As for boolean value of is or is not bookmarked, I'd probably use ternary expression based on $criterion->isBookmarked() getter and call neq or eq depending on the value.

github-actions bot assigned mikadamczyk, alongosz and tischsoic Feb 11, 2025

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from bdac332 to 9164daa Compare February 12, 2025 12:23

vidarl mentioned this pull request Feb 12, 2025

IBX-6773: Bookmarks for non accessible contents cause exception ezsystems/ezplatform-kernel#408

Closed

6 tasks

vidarl added the Ready for review label Feb 12, 2025

vidarl requested a review from a team February 12, 2025 12:28

vidarl added the Doc needed The changes require some documentation label Feb 12, 2025

adamwojs changed the title ~~IBX:6773 bookmarks for non accessible contents cause exception~~ IBX:6773: Bookmarks for non accessible contents cause exception Feb 23, 2025

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 9164daa to ec96060 Compare February 23, 2026 15:43

konradoboza reviewed Feb 24, 2026

View reviewed changes

vidarl commented Feb 24, 2026

View reviewed changes

phpstan-baseline.neon Outdated Show resolved Hide resolved

konradoboza requested a review from a team February 24, 2026 08:56

vidarl commented Feb 24, 2026

View reviewed changes

src/contracts/Persistence/Bookmark/Handler.php Show resolved Hide resolved

ibexa-workflow-automation-1 bot requested review from Steveb-p, ViniTou, alongosz, barw4, ciastektk, konradoboza, mikadamczyk, tbialcz and wiewiurdp and removed request for a team February 24, 2026 08:56

konradoboza reviewed Feb 24, 2026

View reviewed changes

vidarl mentioned this pull request Feb 24, 2026

IBX-6773: Aligned Bookmarks loadBookmarks call with Repository filtering requirements ibexa/admin-ui#1835

Open

vidarl requested a review from konradoboza February 24, 2026 11:42

konradoboza reviewed Feb 25, 2026

View reviewed changes

vidarl and others added 6 commits February 27, 2026 21:02

Fixed PHPStan error for URLAliasService

f015051

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

0bf5649

…n - PHPStand and BookmarkQueryBuilder

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

117d9e2

…n - PHPStand and Bookmark\IdSortClauseQueryBuilder

fixup! IBX-6773: Bookmarks for non-accessible contents cause exceptio…

3837c20

…n - Added more Strict typing

Update tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Loca…

7dfe142

…tion/BookmarkQueryBuilderTest.php Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

[PHPStan] Aligned baseline with the upstream changes

648ac3e

alongosz force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 78b2c3b to 648ac3e Compare February 27, 2026 20:09

konradoboza reviewed Mar 10, 2026

View reviewed changes

tests/integration/Core/Repository/LocationServiceTest.php Outdated Show resolved Hide resolved

konradoboza reviewed Mar 10, 2026

View reviewed changes

src/lib/Repository/BookmarkService.php Outdated Show resolved Hide resolved

Apply suggestions from code review

ba26848

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

vidarl requested a review from konradoboza March 10, 2026 09:39

Added integration test for using Criterion\IsBookmarked and LocationS…

5fd0eb2

…ervice::count()

fixup! Added integration test for using Criterion\IsBookmarked and Lo…

993e850

…cationService::count()

konradoboza approved these changes Mar 10, 2026

View reviewed changes

tests/integration/Core/Repository/BookmarkServiceTest.php Outdated Show resolved Hide resolved

Steveb-p approved these changes Mar 10, 2026

View reviewed changes

fixup! Added integration test for using Criterion\IsBookmarked and Lo…

e436267

…cationService::count()

konradoboza reviewed Mar 10, 2026

View reviewed changes

tests/integration/Core/Repository/BookmarkServiceTest.php Outdated Show resolved Hide resolved

vidarl and others added 2 commits March 10, 2026 14:28

fixup! fixup! Added integration test for using Criterion\IsBookmarked…

d9b18bc

… and LocationService::count()

Apply suggestion from @konradoboza

5343730

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

konradoboza added Ready for QA and removed Ready for review labels Mar 10, 2026

alongosz reviewed Mar 12, 2026

View reviewed changes

vidarl added Ready for review and removed Ready for QA labels Mar 13, 2026

		$this->assertEquals($contactUsLocationId, $afterSwap->items[0]->id);
		$this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id);

Conversation

vidarl commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related PRs:

Description:

For QA:

Documentation:

Uh oh!

sonarqubecloud bot commented Feb 12, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

konradoboza left a comment

Choose a reason for hiding this comment

Uh oh!

vidarl commented Feb 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alongosz commented Feb 27, 2026

Uh oh!

konradoboza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vidarl commented Mar 10, 2026

Uh oh!

konradoboza commented Mar 10, 2026

Uh oh!

konradoboza commented Mar 10, 2026

Uh oh!

vidarl commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Mar 10, 2026

Quality Gate passed

Uh oh!

alongosz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vidarl commented Feb 11, 2025 •

edited

Loading

vidarl commented Mar 10, 2026 •

edited

Loading